Skip to content

Adjust integral term #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 29, 2025
Merged

Adjust integral term #25

merged 1 commit into from
Mar 29, 2025

Conversation

usbalbin
Copy link
Contributor

@usbalbin usbalbin commented Jun 19, 2024 •
edited
Loading

Add methods for getting/setting the integral term.

Add some more tests for integers

@usbalbin
Copy link
Contributor Author

Let me know if you want me to remove the last commit

mtilda
mtilda reviewed Mar 27, 2025
View reviewed changes
Copy link
Collaborator

@mtilda mtilda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be rebased. We added set_integral_term in #27, which pulled a commit from #22.

I like the usage warning in your comment on set_integral_term.

I can see how get_integral_term could be a useful.

The tests you added look good, but they don't seem relevant to the methods you touched. I think they belong in a separate PR.

I rebased your branch locally, dropping the commit with the added tests, and pushed to a branch on my fork (commit).

@usbalbin usbalbin force-pushed the adjust_integral_term branch from a1bc1c2 to 525df5e Compare March 27, 2025 19:34
@usbalbin usbalbin mentioned this pull request Mar 27, 2025
Merged
@usbalbin usbalbin force-pushed the adjust_integral_term branch from 525df5e to 126e3d5 Compare March 27, 2025 19:40
@usbalbin usbalbin force-pushed the adjust_integral_term branch from 126e3d5 to 281b410 Compare March 27, 2025 19:42
@usbalbin
Copy link
Contributor Author

The tests you added look good, but they don't seem relevant to the methods you touched. I think they belong in a separate PR.

Done in #31

I rebased your branch locally, dropping the commit with the added tests, and pushed to a branch on my fork (commit).

Thanks :)

I cherry-picked that commit here. Feel free to merge this or your fork. Fine by me either way

@mtilda mtilda self-requested a review March 28, 2025 20:50
@mtilda mtilda merged commit a1a2d7e into braincore:master Mar 29, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtilda mtilda mtilda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@usbalbin @mtilda